Skip to content

Refactor: Constructor injection, proper exceptions, @Transactional, input validation, SecurityConfig cleanup#174

Open
devin-ai-integration[bot] wants to merge 4 commits intoDevOpsfrom
devin/1777407223-refactor-architecture-code-quality
Open

Refactor: Constructor injection, proper exceptions, @Transactional, input validation, SecurityConfig cleanup#174
devin-ai-integration[bot] wants to merge 4 commits intoDevOpsfrom
devin/1777407223-refactor-architecture-code-quality

Conversation

@devin-ai-integration
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot commented Apr 28, 2026

Summary

Refactors the application architecture and fixes code quality issues across SecurityConfig, BankController, AccountService, and one minimal template fix, without changing endpoints or build tooling.

Changes

  1. Constructor injection — Replaced all field @Autowired with constructor injection in SecurityConfig, BankController, and AccountService. Fields are now private final.

  2. loadUserByUsername() cleanup — Removed the dead if (account == null) check (the preceding findAccountByUsername already throws on not-found). Now queries accountRepository directly and throws UsernameNotFoundException.

  3. Proper exceptions — Replaced generic RuntimeException with ResponseStatusException:

    • NOT_FOUND for "Account not found" and "Recipient account not found"
    • CONFLICT for "Username already exists"
    • BAD_REQUEST for "Insufficient funds" and validation failures
    • Controller uses getReason() (not getMessage()) to extract clean error text for UI display
  4. @Transactional — Added to deposit(), withdraw(), transferAmount(), and registerAccount() to ensure atomicity (especially transferAmount which does multiple DB writes).

  5. Input validation — Deposit/withdraw/transfer amounts validated as positive (> 0). Username and password validated as non-blank during registration.

  6. SecurityConfig simplification — Removed the legacy configureGlobal(AuthenticationManagerBuilder) method. Since AccountService implements UserDetailsService and is a @Service, Spring Security auto-discovers it. Removed the AccountService dependency from SecurityConfig entirely. Changed passwordEncoder() from static to instance method.

  7. Consistent error handling in controller — Added try-catch to the deposit endpoint (was missing, unlike withdraw/transfer). Updated register template to use th:text="${error}" instead of hardcoded "User already present." so new validation errors display correctly.

Review & Testing Checklist for Human

  • Verify login/logout still works — SecurityConfig no longer explicitly wires UserDetailsService; Spring auto-discovers it via AccountService @Service
  • Test deposit, withdraw, and transfer with amount = 0 or negative to confirm validation rejects them
  • Test registration with blank username/password to confirm validation displays correct error
  • Test transfer atomicity — if recipient doesn't exist, sender balance should not change
  • Verify error messages display cleanly on both dashboard and registration pages

Notes

  • No changes to pom.xml or tests
  • One minimal template change: register.html updated to show dynamic error messages (required to match new validation behavior)
  • All REST endpoints and behavior preserved
  • Compilation verified with ./mvnw clean compile -DskipTests

Link to Devin session: https://app.devin.ai/sessions/cb1fa0ebeb6f4536a023113b2ec924c6
Requested by: @Colhodm


Open in Devin Review

- Replace field @Autowired with constructor injection in SecurityConfig,
  BankController, and AccountService
- Fix loadUserByUsername(): remove dead null check, query repository
  directly and throw UsernameNotFoundException
- Replace generic RuntimeException with ResponseStatusException
  (NOT_FOUND, CONFLICT, BAD_REQUEST) throughout AccountService
- Add @transactional to deposit(), withdraw(), transferAmount(),
  and registerAccount()
- Add input validation: positive amount checks for deposit/withdraw/transfer,
  non-blank checks for username/password in registration
- Simplify SecurityConfig: remove legacy configureGlobal() method,
  let Spring Security auto-discover UserDetailsService bean,
  remove unnecessary AccountService dependency

Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
@staging-devin-ai-integration
Copy link
Copy Markdown

Devin Review

Status Commit
⚪ Not started

Open in Devin Review (Staging)

💡 Connect your GitHub account to enable automatic code reviews.

@devin-ai-integration
Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

devin-ai-integration[bot]

This comment was marked as resolved.

…usException

ResponseStatusException.getMessage() in Spring 6.x returns a formatted string
like '400 BAD_REQUEST "Insufficient funds"' including the status code prefix.
Extract the clean reason phrase via getReason() so users see only the human-
readable message.

Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
devin-ai-integration[bot]

This comment was marked as resolved.

The deposit endpoint was missing error handling for the newly added
amount validation in AccountService.deposit(). Without it, invalid
amounts would show a Whitelabel Error Page instead of a user-friendly
error on the dashboard.

Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
devin-ai-integration[bot]

This comment was marked as resolved.

The register template had a hardcoded 'User already present.' message
that was shown for any registration error. With new validation errors
(blank username/password), the hardcoded text was misleading. Now uses
th:text to display the actual error reason from the controller.

Co-Authored-By: Arjun Mishra <arjunsaxmishra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant